Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merging a new entity with PrePersist event make changes in callback not be considered #6174

Conversation

moroine
Copy link

@moroine moroine commented Dec 15, 2016

When merging a new entity, the must act like a basic persist. But it seems that changes made into the PrePersist event callback are not persisted.

To fix this I firstly refactor two features in the doMerge:

  • Added ensureVersionMatch: that check the requested merge entity version is correct (in case of versionned one)
  • Move the isLoaded test used before ̀mergeEntityStateIntoManagedCopyintomergeEntityStateIntoManagedCopy`

Then I move mergeEntityStateIntoManagedCopy calls to ensure that it will be called always before persistNew.

This will also fixed one of my previously PR (#5570) that is schedule for the next patch release.

@moroine
Copy link
Author

moroine commented Dec 15, 2016

@Ocramius it seems tests fail due to Travis

@Ocramius Ocramius self-assigned this Dec 18, 2016
@Ocramius Ocramius self-requested a review December 18, 2016 13:21
@Ocramius Ocramius added this to the 2.5.6 milestone Dec 18, 2016
@Ocramius
Copy link
Member

@moroine everything seems fine in the code changes, but the test is reusing some code that seems unrelated (entity resolver listeners).

I'm currently rewriting the test to get rid of that.

Ocramius added a commit that referenced this pull request Dec 18, 2016
Ocramius added a commit that referenced this pull request Dec 18, 2016
Ocramius added a commit that referenced this pull request Dec 18, 2016
Ocramius added a commit that referenced this pull request Dec 18, 2016
Ocramius added a commit that referenced this pull request Dec 18, 2016
Ocramius added a commit that referenced this pull request Dec 18, 2016
Ocramius added a commit that referenced this pull request Dec 18, 2016
Ocramius added a commit that referenced this pull request Dec 18, 2016
… triggering on `UnitOfWork` into the `UnitOfWorkTest`
Ocramius added a commit that referenced this pull request Dec 18, 2016
…es are merged, but are already in the UoW
Ocramius added a commit that referenced this pull request Dec 18, 2016
Ocramius added a commit that referenced this pull request Dec 18, 2016
Ocramius added a commit that referenced this pull request Dec 18, 2016
…ener`, since `UnitOfWorkTest` now completely encapsulates the scenarios being covered
@Ocramius Ocramius added the Bug label Dec 18, 2016
Ocramius added a commit that referenced this pull request Dec 18, 2016
Ocramius added a commit that referenced this pull request Dec 18, 2016
…es are merged, but are already in the UoW
Ocramius added a commit that referenced this pull request Dec 18, 2016
Ocramius added a commit that referenced this pull request Dec 18, 2016
Ocramius added a commit that referenced this pull request Dec 18, 2016
…ener`, since `UnitOfWorkTest` now completely encapsulates the scenarios being covered
Ocramius added a commit that referenced this pull request Dec 18, 2016
Ocramius added a commit that referenced this pull request Dec 18, 2016
…tities-should-also-trigger-prepersist-lifecycle-callbacks-2.5

Backport #6177 - fix #6174 #5570: merging new entities should also trigger prepersist lifecycle callbacks with the merged data
Ocramius added a commit that referenced this pull request Dec 18, 2016
…tities-should-also-trigger-prepersist-lifecycle-callbacks

Fix #6174 #5570: merging new entities should also trigger prepersist lifecycle callbacks with merged entity data
@Ocramius
Copy link
Member

Moved to #6177, applied fixes locally, rewrote the tests to reduce cross-test dependencies. 👍

@moroine
Copy link
Author

moroine commented Dec 18, 2016

Amazing ! Thanks a lot !

The next patch version will make me abandon my one year fork !!!

AlexKryvets added a commit to AlexKryvets/doctrine2 that referenced this pull request Dec 23, 2016
This release relaxes [`doctrine/common`](https://github.com/doctrine/common) requirements
in order to allow installation of versions that support PHP 7.1 features in proxy class
generation. Please note that a similar requirement relaxation still needs to be applied to
[`doctrine/dbal`](https://github.com/doctrine/dbal) in order to allow installation of
the latest `doctrine/common` versions. [doctrine#6156](doctrine#6156)

This version also backports some fixes around the eviction of the second level cache entries
of inverse side associations in one-to-many - many-to-one mappings. [doctrine#6159](doctrine#6159)

Further fixes were applied in order to have child classes in inheritance mapping share the
same timestamp region when the second level cache is enabled. [doctrine#6028](doctrine#6028)

Also, `Doctrine\ORM\EntityManager#merge()` now triggers `Doctrine\ORM\Events::prePersist`
listeners with the merged entity state whenever an internal `Doctrine\ORM\UnitOfWork#persist()`
call is implied. [doctrine#6177](doctrine#6177).

Total issues resolved: **8**

- [5570: Fix PrePersist EventListener when using merge instead of persist](doctrine#5570)
- [6028: Make child entity share the timestamp region with parent class](doctrine#6028)
- [6110: Clear $this->collection even when empty, to reset keys](doctrine#6110)
- [6156: Allow doctrine/common 2.7](doctrine#6156)
- [6159: doctrine#5821 Backport doctrine#1551 - Fixed support for inverse side second level cache](doctrine#6159)
- [6174: Merging a new entity with PrePersist event make changes in callback not be considered](doctrine#6174)
- [6177: Fix doctrine#6174 doctrine#5570: merging new entities should also trigger prepersist lifecycle callbacks with merged entity data](doctrine#6177)
- [6178: Backport doctrine#6177 - fix doctrine#6174 doctrine#5570: merging new entities should also trigger prepersist lifecycle callbacks with the merged data](doctrine#6178)

# gpg: directory `/c/Users/PC/.gnupg' created
# gpg: new configuration file `/c/Users/PC/.gnupg/gpg.conf' created
# gpg: WARNING: options in `/c/Users/PC/.gnupg/gpg.conf' are not yet active during this run
# gpg: keyring `/c/Users/PC/.gnupg/pubring.gpg' created
# gpg: Signature made Tue Dec 20 00:49:05 2016 FLEST using DSA key ID 12EC2DF8
# gpg: Can't check signature: public key not found

# Conflicts:
#	lib/Doctrine/ORM/Event/LoadClassMetadataEventArgs.php
#	lib/Doctrine/ORM/Tools/Pagination/LimitSubqueryOutputWalker.php
#	tests/Doctrine/Tests/ORM/Functional/QueryCacheTest.php
#	tests/Doctrine/Tests/ORM/Tools/Pagination/LimitSubqueryOutputWalkerTest.php
#	tests/Doctrine/Tests/ORM/Tools/Pagination/PaginationTestCase.php
alexgurrola pushed a commit to Sitetheory-Archive/doctrine2 that referenced this pull request Apr 13, 2017
alexgurrola pushed a commit to Sitetheory-Archive/doctrine2 that referenced this pull request Apr 13, 2017
alexgurrola pushed a commit to Sitetheory-Archive/doctrine2 that referenced this pull request Apr 13, 2017
alexgurrola pushed a commit to Sitetheory-Archive/doctrine2 that referenced this pull request Apr 13, 2017
alexgurrola pushed a commit to Sitetheory-Archive/doctrine2 that referenced this pull request Apr 13, 2017
alexgurrola pushed a commit to Sitetheory-Archive/doctrine2 that referenced this pull request Apr 13, 2017
alexgurrola pushed a commit to Sitetheory-Archive/doctrine2 that referenced this pull request Apr 13, 2017
alexgurrola pushed a commit to Sitetheory-Archive/doctrine2 that referenced this pull request Apr 13, 2017
alexgurrola pushed a commit to Sitetheory-Archive/doctrine2 that referenced this pull request Apr 13, 2017
alexgurrola pushed a commit to Sitetheory-Archive/doctrine2 that referenced this pull request Apr 13, 2017
… event subscriber triggering on `UnitOfWork` into the `UnitOfWorkTest`
alexgurrola pushed a commit to Sitetheory-Archive/doctrine2 that referenced this pull request Apr 13, 2017
…lled when entities are merged, but are already in the UoW
alexgurrola pushed a commit to Sitetheory-Archive/doctrine2 that referenced this pull request Apr 13, 2017
alexgurrola pushed a commit to Sitetheory-Archive/doctrine2 that referenced this pull request Apr 13, 2017
alexgurrola pushed a commit to Sitetheory-Archive/doctrine2 that referenced this pull request Apr 13, 2017
alexgurrola pushed a commit to Sitetheory-Archive/doctrine2 that referenced this pull request Apr 13, 2017
alexgurrola pushed a commit to Sitetheory-Archive/doctrine2 that referenced this pull request Apr 13, 2017
…panyContractListener`, since `UnitOfWorkTest` now completely encapsulates the scenarios being covered
alexgurrola pushed a commit to Sitetheory-Archive/doctrine2 that referenced this pull request Apr 13, 2017
…or PHP 5.4 compat (still supported in ORM 2.5.x)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants